feat: only display panics when in debug mode #648
Conversation
WalkthroughThe pull request introduces modifications to the project's routing and middleware handling across multiple files. In the Changes
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #648 +/- ##
==========================================
- Coverage 81.69% 81.55% -0.15%
==========================================
Files 131 131
Lines 7070 7059 -11
==========================================
- Hits 5776 5757 -19
- Misses 991 999 +8
Partials 303 303 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/api/router.go (1)
49-73: Enhance panic recovery with structured error handling.While the panic recovery logic correctly implements the PR objective of only showing stack traces in debug mode, consider these improvements:
- Add structured error response in JSON format
- Add structured logging of panics
- Consider different handling for different panic types
- Consider security implications of exposing stack traces
func(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { defer func() { if rvr := recover(); rvr != nil { if rvr == http.ErrAbortHandler { panic(rvr) } + span := trace.SpanFromContext(r.Context()) + err := fmt.Errorf("%v", rvr) + span.RecordError(err) + if debug { middleware.PrintPrettyStack(rvr) } - trace.SpanFromContext(r.Context()).RecordError(fmt.Errorf("%s", rvr)) - w.WriteHeader(http.StatusInternalServerError) + w.Header().Set("Content-Type", "application/json") + errorResponse := map[string]interface{}{ + "error": "Internal Server Error", + "code": "INTERNAL_ERROR", + } + if debug { + errorResponse["debug"] = fmt.Sprintf("%v", rvr) + } + json.NewEncoder(w).Encode(errorResponse) } }() next.ServeHTTP(w, r) } return http.HandlerFunc(fn) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Justfile(1 hunks)internal/api/router.go(3 hunks)internal/api/v1/routes.go(1 hunks)internal/api/v2/routes.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/api/v2/routes.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (4)
Justfile (1)
7-7: LGTM! Good addition of a shorter alias.The addition of the
pcalias for thepre-committarget improves developer experience by providing a shorter command.internal/api/v1/routes.go (1)
81-81: LGTM! Good cleanup of router options.The simplification of
routerOptionsto only keep the essentialtracerfield aligns with the removal of custom middleware handling.internal/api/router.go (2)
48-48: LGTM! Good addition of OTLP middleware.The addition of OpenTelemetry middleware enhances observability.
104-105: LGTM! Good field reordering.The reordering of fields in
routerOptionsimproves readability.
gfyrag
force-pushed
the
feat/only-display-panics-on-debug
branch
from
c10e8f0 to
8072096
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/api/v1/routes.go (1)
Line range hint
37-41: Remove unnecessary middleware.This middleware function is a pass-through that adds no functionality. Consider removing it to simplify the middleware chain and improve performance.
- router.Use(func(handler http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - handler.ServeHTTP(w, r) - }) - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Justfile(1 hunks)internal/api/router.go(3 hunks)internal/api/v1/routes.go(1 hunks)internal/api/v2/routes.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/api/v2/routes.go
🚧 Files skipped from review as they are similar to previous changes (2)
- Justfile
- internal/api/router.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
internal/api/v1/routes.go (1)
81-81: Verify panic handling configuration.With the removal of custom middleware support, ensure that panic handling is properly configured at a higher level (e.g., in the main router or recovery middleware) to control panic displays based on the debug mode.
Let's verify the panic handling configuration:
✅ Verification successful
Panic handling is properly configured
The panic handling is correctly implemented at the router level with debug mode awareness through
middleware.PrintPrettyStack. The removal of custom middleware support fromrouterOptionsdoesn't impact this functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for panic handling configuration in the main router or recovery middleware. # Look for panic handling or recovery middleware rg -A 5 'recovery|panic|debug.*mode' internal/api/ # Look for debug mode configuration rg -A 5 'debug.*=|debug.*bool' internal/api/Length of output: 2266
No description provided.